-
Notifications
You must be signed in to change notification settings - Fork 773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix TFM bugs #4912
Fix TFM bugs #4912
Conversation
@@ -77,7 +77,6 @@ | |||
<PackageVersion Include="RabbitMQ.Client" Version="[6.5.0,7.0)" /> | |||
<PackageVersion Include="StyleCop.Analyzers" Version="[1.2.0-beta.507,2.0)" /> | |||
<PackageVersion Include="Swashbuckle.AspNetCore" Version="[6.4.0]" /> | |||
<PackageVersion Include="System.Net.Http" Version="4.3.4" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be used at all since it's a dead package https://www.nuget.org/packages/System.Net.Http/ (no updates since year 2018).
Codecov Report
@@ Coverage Diff @@
## main open-telemetry/opentelemetry-dotnet#4912 +/- ##
==========================================
+ Coverage 83.30% 83.51% +0.20%
==========================================
Files 295 295
Lines 12294 12294
==========================================
+ Hits 10242 10267 +25
+ Misses 2052 2027 -25
Flags with carried forward coverage won't be shown. Click here to find out more. |
docs/trace/getting-started-jaeger/getting-started-jaeger.csproj
Outdated
Show resolved
Hide resolved
@@ -15,7 +15,6 @@ | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="System.Net.Http" Condition="'$(TargetFramework)' == 'net462'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve this but FYI what I really want is for the tests to verify everything works if the user decides to take the NuGet bits. But we should also make sure it works with the direct reference. Probably need to run the tests through both modes to do it correctly? Going to take a bit of effort to pull that off, not sure if the juice is worth the squeeze.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR, the test is going to cover the net462 framework reference of System.Net.Http
, do you think such test coverage can represent NuGet System.Net.Http
(especially considering that the NuGet hasn't been updated since 2018) or we need both?
what I really want is for the tests to verify everything works if the user decides to take the NuGet bits
Could you explain a bit more here? What's the main scenario and how big is the user base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely old, but I don't see anything on the NuGet saying it is deprecated and it has been downloaded 1.7B times so a lot of people are consuming it! The scenario is user takes a dependency on the NuGet and its bits are used instead of the shipped runtime assembly,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4915
Fix two issues:
!= true
instead of== false
since in certain environments the property doesn't exist.System.Net.Http
reference should be coming from framework instead of package, @cijothomas explained to me that https://www.nuget.org/packages/System.Net.Http/ nuget should not be used at all because it's abandoned since year 2018.